-
Notifications
You must be signed in to change notification settings - Fork 113
[oneDPL] Add memory parallel range algorithms #631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[oneDPL] Add memory parallel range algorithms #631
Conversation
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
// Extension of C++20 exposition-only special memory concepts as defined in [special.mem.concepts] | ||
template<typename S, typename I> | ||
concept no-throw-sized-sentinel-for = // exposition only | ||
no-throw-sentinel-for<S, I> && | ||
std::sized_sentinel_for<S, I>; | ||
|
||
template<typename I> | ||
concept no-throw-bidirectional-iterator = // exposition only | ||
no-throw-forward-iterator<I> && | ||
std::bidirectional_iterator<I>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think defining 7 exposition-only concepts (yes, technically two more are needed - for sentinels and forward iterators), of which only one is used in the algorithm signatures, is an overkill.
Perhaps I would only add no-throw-sized-random-access-range
as the combination of sized and random access ranges with additional semantical requirements described by words.
Tagging @rarutyun for another opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, based on the discussion at uxlfoundation/oneDPL@cb303af#r2109002570, it is important to specifically add the requirement for the result of dereferencing to be an lvalue reference to the iterator value type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is not it sufficient to have std::is_lvalue_reference_v<std::iter_reference_t<I>>
already requested in no-throw-input-iterator
?
See:
template< class I >
concept no-throw-input-iterator =
std::input_iterator<I> &&
std::is_lvalue_reference_v<std::iter_reference_t<I>> &&
std::same_as<std::remove_cvref_t<std::iter_reference_t<I>>, std::iter_value_t<I>>;
Upd. I suppose you suggest adding std::is_lvalue_reference_v<std::iter_reference_t<I>>
as a part of no-throw-sized-random-access-range
. Let me try...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented your suggestion in b69247a. Could you check it?
What I did differently:
- The concept does not contain "sized" part for consistency with other algorithms.
- It is renamed to "nothrow" from "no-throw" to follow C++23 naming because it does not depend on the C++20 concepts now.
- I added
is_lvalue_reference_v
andsame_as
as I assume both are needed for "add the requirement for the result of dereferencing to be an lvalue reference to the iterator value type". - I added
sized_sentinel_for
because it remains if I substitute each syntactic requirement (it's a part ofnothrow-sentinel-for
).
The list of semantic requirements inherits the properties of nothrow-sentinel-for
, nothrow-bidirectional-iterator
, nothrow-random-access-iterator
and nothrow-random-access-range
from https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2025/p3179r9.html#modify_special_mem_concepts and the base concepts from https://en.cppreference.com/w/cpp/memory/ranges/nothrow_concepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, as I check it against the paper, with this discussion in mind, I notice that here we use: std::sized_sentinel_for<std::ranges::sentinel_t<R>, std::ranges::iterator_t<R>>
rather than
std::sized_sentinel_for<std::ranges::iterator_t<R>, std::ranges::iterator_t<R>>
which I think is what we end up with when combining the concepts in the paper.
It does makes more sense to provide the sentinel of the range to sized_sentinel_for
as first argument rather than the iterator of the range, but I'm not sure if we are adding any extra requirements beyond what is proposed in the paper, so I wanted to note it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It's a mistake.
sized_sentinel_for<I, I>
is a valid requirement for an iterator. random_access_range and consequently random_access_iterator
has it.
Here is a description of sized_sentinel_for' for more context:
The sized_sentinel_for concept specifies that an object of the iterator type I and an object of the sentinel type S can be subtracted to compute the distance between them in constant time.
Two random access iterators with the same type must provide the distance when subtracted from each other. So, even if std::sized_sentinel_for
has sentinel in its name, we must use std::sized_sentinel_for<I, I>
.
Thinking further, nothrow-random-access-iterator
, which is a part of nothrow-random-access-range
, has nothrow-sized-sentinel-for
only to add "no-throwness". We describe that trait separately in "Semantic Requirements", so there is not need in std::sized_sentinel_for<I, I>
at all - it's already included into std::random_access_iterator
. I am removing this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I would rather not introduce our own concepts and just make a reference to the current C++ working draft where all those "exposition only" concepts are defined (assuming we have no difference with them). The stable link is this one: https://eel.is/c++draft/special.mem.concepts, which I would make as [special.mem.concepts] to appear in our specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine to add a note about near-equivalence to the C++26 working draft. But I prefer the "normative" definitions in the specification to be based on C++20.
Updated: added a general note for that (not just about the concept) in #636.
6378beb
to
0dc211c
Compare
0dc211c
to
b69247a
Compare
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Dan Hoeflinger <[email protected]>
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looking at the new set of Parallel Range Algorithms I am pretty sure that either referring to the C++ working draft or adding execution-policy
concept to our spec would simplify the signatures quite significantly...
using /*projected-value-type*/ = std::remove_cvref_t<std::invoke_result_t<Proj&, std::iter_value_t<I>&>>; | ||
|
||
// C++20 analogue of nothrow-random-access-range proposed for C++26 in P3179R9; exposition only | ||
template <typename R> | ||
concept nothrow-random-access-range = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we decide to "copy" this concept instead of nothrow-sized-random-access-range
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for consistency with the other signatures, where sized_range
comes under requires
clauses.
Co-authored-by: Alexey Kukanov <[email protected]>
source/elements/oneDPL/source/parallel_api/parallel_range_api.rst
Outdated
Show resolved
Hide resolved
// uninitialized_fill | ||
template <typename ExecutionPolicy, /*nothrow-random-access-range*/ R, typename T> | ||
requires oneapi::dpl::is_execution_policy_v<std::remove_cvref_t<ExecutionPolicy>> && | ||
std::ranges::sized_range<R> && | ||
std::constructible_from<std::ranges::range_value_t<R>, const T&> | ||
std::ranges::borrowed_iterator_t<R> | ||
uninitialized_fill (ExecutionPolicy&& pol, R&& r, const T& value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For uninitialized_fill
we should also add the default type for typename T
, similarly to fill
. According to @rarutyun this was inadvertently forgotten by the author of that proposal, and we have not spotted it in P3179, only later.
We cannot reuse |
Adding
uninitialized_default_construct
,uninitialized_value_construct
,uninitialized_move
,uninitialized_copy
,uninitialized_fill
anddestroy
parallel range algorithms into oneDPL specification.